[types] Add type definitions from DefinitelyTyped/DefinitelyTyped#62510 #221

MysteryBlokHed · 2022-10-02T20:16:50Z

Closes #119

Moves types from the original DefinitelyTyped PR and adds some dependencies and workflows to test them.

This PR is still very much a work in progress, and I'm pretty sure there are some parts of the API that I haven't typed yet.

Moves types from the original DefinitelyTyped PR and adds some dependencies and workflows to test them. These changes are super broken on my machine right now and I can't figure out why, but I'll try to get it fixed.

LeaVerou · 2022-10-03T15:58:21Z

Wow, that is a very substantial amount of work, thank you so much @MysteryBlokHed!

I'm a bit worried I lack the TS experience to review this properly, perhaps someone else could take a look when it's ready? @ai @jgerigmeyer would that be of interest to you?

jgerigmeyer · 2022-10-03T17:03:19Z

@jgerigmeyer would that be of interest to you?

Yes, I'd be happy to take a look at this! I should be able to do that in the next few days. 👍

MysteryBlokHed · 2022-10-05T01:15:03Z

I think most of the API has types now. I just need to figure out the best way to add types for the space accessors

jgerigmeyer

This is so helpful -- thanks @MysteryBlokHed! 🎉

I wasn't able to get dtslint running locally without some further changes -- submitted in MysteryBlokHed#1

I pulled this in to a personal TypeScript project that uses colorjs.io (replacing some type stubs I'd written there), and there were only a few places where it seems these types were slightly incorrect -- I submitted a first pass at some changes in MysteryBlokHed#2

* Initial round of types review * Address review

jgerigmeyer

🚀

LeaVerou · 2022-10-13T18:29:19Z

@jgerigmeyer thank you so much! I just added you as a collaborator.

@MysteryBlokHed @jgerigmeyer is this ready to merge?

LeaVerou · 2022-10-13T18:30:52Z

package.json

+		"src/",
+		"types/dist/",
+		"types/src/",
+		"types/index.d.ts"


Why did we choose this structure over src/types and dist/types? Is that the convention? (no problem if that's the case, just thought I'd ask)

I just kept this structure when I copied the types over from DefinitelyTyped, but I think doing it this way also makes testing easier since everything's in the same place

LeaVerou · 2022-10-13T18:32:34Z

.github/workflows/test-types.yml

+      - uses: actions/checkout@v3
+      - uses: actions/setup-node@v3
+        with:
+          node-version: "12"


Isn't 12 a bit old at this point?

I think I chose 12 because the Babel config specifies version <=12, but it could be made more recent

LeaVerou · 2022-10-13T18:32:45Z

.gitignore

@@ -1,4 +1,4 @@
-dist/
+/dist/


Adding the leading slash means that Git will only ignore the dist folder in the root of the project. Without this change, the folder types/dist is also ignored

LeaVerou · 2022-10-13T18:36:05Z

I had a quick look and left some questions.

I see there is also a new test directory with different tests. How does this compare to the existing tests? Are they complementary? Is it primarily testing types? We should probably document why there are two different sets of tests somewhere, for new contributors?

jgerigmeyer · 2022-10-13T18:40:43Z

I'm 99% sure that we'll discover small issues once people start actually using this with TypeScript, but IMO this is already a significant improvement and is ready to merge. 🎉

It would be much easier to maintain these types going forward if the codebase itself were re-written in TypeScript, but I understand that's not a simple change and wouldn't have to happen right away regardless.

MysteryBlokHed · 2022-10-13T18:41:59Z

I see there is also a new test directory with different tests. How does this compare to the existing tests? Are they complementary? Is it primarily testing types? We should probably document why there are two different sets of tests somewhere, for new contributors?

The tests in the types directory only test the definitions; they're not actually evaluated like the existing tests.

A mention of the reason for the two directories could be mentioned in the README or CONTRIBUTING files maybe? I could make a change when I'm free in a few hours

LeaVerou · 2022-10-13T18:45:42Z

I'm 99% sure that we'll discover small issues once people start actually using this with TypeScript, but IMO this is already a significant improvement and is ready to merge. 🎉

Ok, I plan to rebase and merge soon, so if there's anything you want to squash in the commit log, now's the time.

It would be much easier to maintain these types going forward if the codebase itself were re-written in TypeScript, but I understand that's not a simple change and wouldn't have to happen right away regardless.

I know, but I don't want to tie the source to a build tool, it's nice that right now things like this work in a browser: import Color from "https://colorjs.io/src/color.js". I think having separate typing files is a nice compromise, though they do double the number of files 😕

LeaVerou · 2022-10-13T18:47:41Z

I see there is also a new test directory with different tests. How does this compare to the existing tests? Are they complementary? Is it primarily testing types? We should probably document why there are two different sets of tests somewhere, for new contributors?

The tests in the types directory only test the definitions; they're not actually evaluated like the existing tests.

A mention of the reason for the two directories could be mentioned in the README or CONTRIBUTING files maybe? I could make a change when I'm free in a few hours

I totally missed they were in types. I think that's pretty self-explanatory, though of course you're welcome to add something in CONTRIBUTING.md if you feel like it.

jgerigmeyer · 2022-10-13T18:53:22Z

Ok, I plan to rebase and merge soon, so if there's anything you want to squash in the commit log, now's the time.

I'm not particular about a "clean" git history, so I'll defer to @MysteryBlokHed on whether to squash before merging.

MysteryBlokHed · 2022-10-13T19:09:36Z

Rebasing seems fine and I don't think there's anything else that needs changing right now

LeaVerou · 2022-10-13T19:40:30Z

Merged, thank you both!

1nfinity-5starZ · 2022-10-21T15:49:39Z

Was this shipped in latest release ?

LeaVerou · 2022-10-21T17:00:09Z

No, it will be in the next release (likely v0.4.1)

This was referenced Oct 2, 2022

Typescript support #119

Closed

[colorjs.io] Add types for colorjs.io DefinitelyTyped/DefinitelyTyped#62510

Closed

[types] Fix problems highlighted by dtslint

936e9af

jgerigmeyer requested changes Oct 11, 2022

View reviewed changes

jgerigmeyer added 2 commits October 11, 2022 15:49

Fix dtslint (#1)

ef28db5

Initial round of types review (#2)

89d1da2

* Initial round of types review * Address review

jgerigmeyer approved these changes Oct 11, 2022

View reviewed changes

[types] Add types for space accessors

138d5f1

MysteryBlokHed marked this pull request as ready for review October 11, 2022 21:50

LeaVerou reviewed Oct 13, 2022

View reviewed changes

LeaVerou merged commit efcd444 into color-js:main Oct 13, 2022

MysteryBlokHed deleted the types/add-definitions branch October 21, 2022 21:37

LeaVerou mentioned this pull request Nov 3, 2022

How to generate old syntax for rgba for CSS? #203

Closed

MysteryBlokHed mentioned this pull request Nov 3, 2022

[ci] Update conditions for dtslint workflow run #236

Merged

Olian04 mentioned this pull request Nov 9, 2022

Re-Implement color manipulation logic Olian04/Pelle#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[types] Add type definitions from DefinitelyTyped/DefinitelyTyped#62510 #221

[types] Add type definitions from DefinitelyTyped/DefinitelyTyped#62510 #221

MysteryBlokHed commented Oct 2, 2022 •

edited

Loading

LeaVerou commented Oct 3, 2022

jgerigmeyer commented Oct 3, 2022

MysteryBlokHed commented Oct 5, 2022

jgerigmeyer left a comment

jgerigmeyer left a comment

LeaVerou commented Oct 13, 2022

LeaVerou Oct 13, 2022

MysteryBlokHed Oct 13, 2022

LeaVerou Oct 13, 2022

MysteryBlokHed Oct 13, 2022 •

edited

Loading

LeaVerou Oct 13, 2022

MysteryBlokHed Oct 13, 2022

LeaVerou commented Oct 13, 2022

jgerigmeyer commented Oct 13, 2022

MysteryBlokHed commented Oct 13, 2022 •

edited

Loading

LeaVerou commented Oct 13, 2022

LeaVerou commented Oct 13, 2022

jgerigmeyer commented Oct 13, 2022

MysteryBlokHed commented Oct 13, 2022

LeaVerou commented Oct 13, 2022

1nfinity-5starZ commented Oct 21, 2022

LeaVerou commented Oct 21, 2022

		@@ -1,4 +1,4 @@
		dist/
		/dist/

[types] Add type definitions from DefinitelyTyped/DefinitelyTyped#62510 #221

[types] Add type definitions from DefinitelyTyped/DefinitelyTyped#62510 #221

Conversation

MysteryBlokHed commented Oct 2, 2022 • edited Loading

LeaVerou commented Oct 3, 2022

jgerigmeyer commented Oct 3, 2022

MysteryBlokHed commented Oct 5, 2022

jgerigmeyer left a comment

Choose a reason for hiding this comment

jgerigmeyer left a comment

Choose a reason for hiding this comment

LeaVerou commented Oct 13, 2022

LeaVerou Oct 13, 2022

Choose a reason for hiding this comment

MysteryBlokHed Oct 13, 2022

Choose a reason for hiding this comment

LeaVerou Oct 13, 2022

Choose a reason for hiding this comment

MysteryBlokHed Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

LeaVerou Oct 13, 2022

Choose a reason for hiding this comment

MysteryBlokHed Oct 13, 2022

Choose a reason for hiding this comment

LeaVerou commented Oct 13, 2022

jgerigmeyer commented Oct 13, 2022

MysteryBlokHed commented Oct 13, 2022 • edited Loading

LeaVerou commented Oct 13, 2022

LeaVerou commented Oct 13, 2022

jgerigmeyer commented Oct 13, 2022

MysteryBlokHed commented Oct 13, 2022

LeaVerou commented Oct 13, 2022

1nfinity-5starZ commented Oct 21, 2022

LeaVerou commented Oct 21, 2022

MysteryBlokHed commented Oct 2, 2022 •

edited

Loading

MysteryBlokHed Oct 13, 2022 •

edited

Loading

MysteryBlokHed commented Oct 13, 2022 •

edited

Loading